Fix: Instrumented tests are executed in Drone CI again#16612
Fix: Instrumented tests are executed in Drone CI again#16612
Conversation
|
|
||
| debug { | ||
| enableUnitTestCoverage = project.hasProperty("coverage") | ||
| enableAndroidTestCoverage = project.hasProperty("coverage") |
There was a problem hiding this comment.
@tobiasKaminsky with this, Drone now actually runs the instrumented tests again. See https://drone.nextcloud.com/nextcloud/android/28585/1/3 as a proof of failure, i.e. broken tests on master which were never detected.
I'll go ahead addressing the test failures. But there is several other things I noticed:
- We should have Drone only run the instrumentation tests. Currently, drone CI runs everything for which coverage is enabled. Why this is chosen as filter, I don't know. But what this means is that Drone wastes time on the unit tests which we already run elsewhere.
- Unit tests were the only tests Drone ran since Chore: Improve Gradle Configuration #15859. Another weird side-effect of that PR.
- Easiest for saving drone time would be to disable the coverage for unit tests in the line above, but I guess this is not desired?
- Actually, what about the coverage reporting? It doesn't appear anymore. It looks broken, in several ways: - I am not seeing the coverage report on any PR I have recently checked. Don't know when this stopped. Many of the pulls on Codecov just show errors: https://app.codecov.io/github/nextcloud/android/pulls
- The unit test job seems to still upload to Codecov, but prints out wrong links in the log, e.g.https://app.codecov.io/github/***/android/commit/2c1438fba2fda500d3ec2e330d2aa62284ef8e2c (log) .
- You need to replace the
***withnextcloud, then it works. Is this redaction of secrets at play - if so, I surely hope, no secret token has simply "nextcloud" as value?
- Coverage reporting should also be extended by the instrumentation tests. I don't see them uploading anything
| if [ $TYPE = "IT" ]; then | ||
| FOLDER=app/build/reports/androidTests/connected/flavors/gplay | ||
| FOLDER=app/build/reports/androidTests/connected/debug/flavors/gplay |
There was a problem hiding this comment.
@tobiasKaminsky these adjustments are needed so the report files are actually found. But reporting still is broken, which you can see e.g. in the log of https://drone.nextcloud.com/nextcloud/android/28585/1/3:
It does upload the report to https://www.kaminsky.me/nc-dev/android-integrationTests/28585-IT-stable-10-44 but it fails to create the expected Github comment notifying the developer of this.
The error is:
{
"message": "Must have admin rights to Repository.",
"documentation_url": "https://docs.github.com/rest/issues/comments#create-an-issue-comment",
"status": "403"
}
d904e65 to
ffaabe4
Compare
The Drone CI job is running ./gradlew createGplayDebugCoverageReport This only executes tests with coverage. In 5fd2e29, the line testCoverageEnabled = project.hasProperty("coverage") was replaced with enableUnitTestCoverage = project.hasProperty("coverage") which only left the unit tests with coverage Enabling the coverage for androidTest (the instrumented tests) makes Drone run them again. Closes #16350 Signed-off-by: Philipp Hasper <vcs@hasper.info>
The drone tests simply ended with the log line: stable-IT test failed, but no output was generated. Maybe a preliminary stage failed. The reason was that the script didn't use the correct folder Signed-off-by: Philipp Hasper <vcs@hasper.info>
1. SetOnlineStatusBottomSheetIT: Checked for the wrong view, at the wrong point in time 2. SetStatusMessageBottomSheetIT: Fragment transactions are scheduled asynchronously, so you couldn't set the status inside the same scenario where show() is shown. Now, the setPredefinedStatus() method instead pre-stores the status and only updates the adapter if available Signed-off-by: Philipp Hasper <vcs@hasper.info>
The tests themselves run through, but immediately after the android process crashes. Bluntly mocking the whole System class causes problems everywhere. Even limiting the mocking to a single method still causes the crashes. The issue is that mocking System.currentTimeMillis() on Android (ART runtime) is fundamentally unsafe, even with a function reference. The mock interferes with framework internals (WorkManager, schedulers, etc.) that run after the test completes, causing the process crash. Signed-off-by: Philipp Hasper <vcs@hasper.info>
ffaabe4 to
f0ee559
Compare
| context = InstrumentationRegistry.getInstrumentation().context | ||
| MockKAnnotations.init(this, relaxed = true) | ||
| mockkStatic(System::class) | ||
| mockkStatic(System::currentTimeMillis) |
There was a problem hiding this comment.
@alperozturk96 see the commit message for more details:
fix(test): Futile attempt at fixing the UploadDateTests crash
The tests themselves run through, but immediately after the android process
crashes. Bluntly mocking the whole System class causes problems everywhere.
Even limiting the mocking to a single method still causes the crashes.
The issue is that mocking System.currentTimeMillis() on Android
(ART runtime) is fundamentally unsafe, even with a function reference.
The mock interferes with framework internals (WorkManager, schedulers,
etc.) that run after the test completes, causing the process crash.
You should also see this test crash on your local machine. The test itself is green, but the whole test run is failed:
And the same now happens on the Drone job, now that it correctly picks up all expected tests.
In the best case, the UploadDateTests should be a unit test, instead of an instrumented test. There, I expect the mocking of currentTimeMillis() to not interfere with outside processes. But in the git history I saw that you initially even had that test as unit test and then moved it over. As it is not explained in the commits, I don't know why.
Does it have to do with the context being required for the R.string and for DateUtils.getRelativeDateTimeString?
We could mock it using Robolectric, but that would be a new dependency I don't want to introduce into the repo without consultation.
The test-stable failed in the skipAutoRenameWhenWCFDisabled test, while test-master succeeded. This is due to the different server versions. At the time of writing, stable is using version 30 and master is using version 32. The WCF handling differs per version, as detailed in the OCCapabilityExtensions.kt file. Hence the test needs to adjust the server version. While we are on it, we are testing the three different cases Signed-off-by: Philipp Hasper <vcs@hasper.info>
|
APK file: https://www.kaminsky.me/nc-dev/android-artifacts/16612.apk |
|
blue-Light-Screenshot test failed, but no output was generated. Maybe a preliminary stage failed. |

The Drone CI job is running ./gradlew createGplayDebugCoverageReport This only executes tests with coverage.
In 5fd2e29 (PR #15859), the line
was replaced with
which only left the unit tests with coverage
Enabling the coverage for androidTest (the instrumented tests) makes Drone run them again.
Closes #16350
🏁 Checklist